Skip to content

[LLD][ELF] Skip non-SHF_ALLOC sections when checking max VA and max VA difference in relaxOnce() #145863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Enna1
Copy link
Contributor

@Enna1 Enna1 commented Jun 26, 2025

For non-SHF_ALLOC sections, sh_addr is set to 0.
Skip sections without SHF_ALLOC flag, so minVA will not be set to 0 with non-SHF_ALLOC sections, and the size of non-SHF_ALLOC sections will not contribute to maxVA.

…A difference in relaxOnce()

For non-SHF_ALLOC sections, sh_addr is set to 0.
Skip sections without SHF_ALLOC flag, so `minVA` will not be set to 0 with
non-SHF_ALLOC sections, and the size of non-SHF_ALLOC sections will not
contribute to `maxVA`.
@llvmbot
Copy link
Member

llvmbot commented Jun 26, 2025

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Mingjie Xu (Enna1)

Changes

For non-SHF_ALLOC sections, sh_addr is set to 0.
Skip sections without SHF_ALLOC flag, so minVA will not be set to 0 with non-SHF_ALLOC sections, and the size of non-SHF_ALLOC sections will not contribute to maxVA.


Full diff: https://github.com/llvm/llvm-project/pull/145863.diff

1 Files Affected:

  • (modified) lld/ELF/Arch/X86_64.cpp (+2)
diff --git a/lld/ELF/Arch/X86_64.cpp b/lld/ELF/Arch/X86_64.cpp
index 163505102d0ec..488f4803b2cb4 100644
--- a/lld/ELF/Arch/X86_64.cpp
+++ b/lld/ELF/Arch/X86_64.cpp
@@ -320,6 +320,8 @@ bool X86_64::deleteFallThruJmpInsn(InputSection &is, InputFile *file,
 bool X86_64::relaxOnce(int pass) const {
   uint64_t minVA = UINT64_MAX, maxVA = 0;
   for (OutputSection *osec : ctx.outputSections) {
+    if (!(osec->flags & SHF_ALLOC))
+      continue;
     minVA = std::min(minVA, osec->addr);
     maxVA = std::max(maxVA, osec->addr + osec->size);
   }

@MaskRay
Copy link
Member

MaskRay commented Jun 26, 2025

This is correct but can you edit /x86-64-gotpc-relax-too-far.s to make this testable?

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@Enna1
Copy link
Contributor Author

Enna1 commented Jun 27, 2025

Thanks for the review!

This is correct but can you edit /x86-64-gotpc-relax-too-far.s to make this testable?

Tried but didn't make it...

The max VA and max VA difference checks are for early return.
The real decision for not relaxing R_X86_64_(REX_)GOTPCRELX is made by checking !isInt<32>(getRelocTargetVA).
Even if max VA or max VA difference in -pie/-shared is >= 2^31 (say the size of .strtab section is 2^31), isInt<32>(getRelocTargetVA) still can be true and R_X86_64_(REX_)GOTPCRELX will be relaxed.
I thinks current implementation doesn't have correctnees issue, but this change can remove some redundant isInt<32>(getRelocTargetVA) checks, so I didn't come up how to extend /x86-64-gotpc-relax-too-far.s to test this.

Do you have any suggestions? Thanks!

@MaskRay
Copy link
Member

MaskRay commented Jun 27, 2025

You are right that the SHF_ALLOC condition just skips some check and does not change the behavior. Then why does it matter? Do you find a scenario where having the condition improves performance?

@Enna1
Copy link
Contributor Author

Enna1 commented Jun 27, 2025

I'm looking for strategies that alleviate relocation overflow pressure. While trying to understand 9d6ec28 and f3c4dae, I found the max VA difference takes non-SHF_ALLOC sections into account, which is a bit confusing.

Do you find a scenario where having the condition improves performance?

I will test with internal cases to see if there is any performance improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants